Codecov Report

❌ Patch coverage is 64.93506% with 27 lines in your changes missing coverage. Please review.

Flags with carried forward coverage won't be shown. Click here to find out more.

... and 1 file with indirect coverage changes

@claude review it

Claude finished @AnnatarHe's task in 2m 35s —— View job

Claude Code Review

• Gather context from PR description and review comments
• Read changed files (commands/daemon.install.go, model/daemon_fetch.go, model/daemon_fetch_test.go, model/updater.go)
• Analyze the implementation and identify issues
• Post review feedback

Overall: the self-heal flow is the right shape — fast-path on ResolveDaemonBinaryPath, refuse to clobber Homebrew installs, normalize tags, and reuse the existing update.go plumbing. Tests are thorough for the new model/daemon_fetch.go. The biggest concerns are real but small: a security trade-off on checksum failures, a brittle error-string match, and zero coverage on the commands/ orchestration. Nothing is a blocker.

Findings

1. FetchChecksum errors are silently swallowed — security regression vs. shelltime update  _blocking-ish

model/daemon_fetch.go:74:

sum, _, _ := FetchChecksum(ctx, tag, archiveName)
// FetchChecksum errors and absent checksums are both non-fatal; we proceed
// without verification, matching the `shelltime update` behavior.

The comment isn't quite right. FetchChecksum distinguishes three cases:

• 404 → ("", false, nil) (checksum doesn't exist for this release — expected for old releases)
• 200 with parse → (sha, true, nil)
• network error / 5xx / malformed body → ("", false, err)

By throwing away the error, you collapse case 3 into case 1. A MITM (or a transient 502 from GitHub) silently downgrades you to unverified download. The narrow fix is to treat only (_, false, nil) as "skip verification" and surface the other errors:

sum, found, err := FetchChecksum(ctx, tag, archiveName)
if err != nil {
    return "", fmt.Errorf("fetch checksum: %w", err)
}
if !found {
    // legitimately absent (old release) — proceed unverified, but warn
}

Fix this →

2. strings.Contains(err.Error(), "status 404") is brittle  _{nit, agree with gemini}

model/daemon_fetch.go:58 and the producer at model/updater.go:228 ("download %s returned status %d"). One typo in a future cleanup of the error string and the fallback silently stops working. Easy hardening:

// updater.go
var ErrAssetNotFound = errors.New("release asset not found")

if resp.StatusCode == http.StatusNotFound {
    return fmt.Errorf("%w: %s", ErrAssetNotFound, url)
}

Then errors.Is(err, ErrAssetNotFound) at the callsite. Same change is worth applying to FetchLatestVersion if you want to surface "no releases yet" cleanly.

Fix this →

3. ResolveCLIBinaryPath() error swallowed  _{agree with gemini, minor}

commands/daemon.install.go:49: if os.Executable() errors, cliPath == "" and the Homebrew guard in EnsureDaemonBinary is bypassed — we could end up dropping a binary into ~/.shelltime/bin for a user whose real binary lives under /opt/homebrew/. Unlikely in practice but cheap to log:

cliPath, err := model.ResolveCLIBinaryPath()
if err != nil {
    color.Yellow.Printf("⚠️  could not resolve CLI path (%v) — Homebrew detection may be inaccurate\n", err)
}

4. The latest-tag fallback discards the second error  _nit

model/daemon_fetch.go:58-66:

if destPath, ferr := fetchDaemonToCurlPath(ctx, latest, archiveName); ferr == nil {
    return destPath, nil
}
// ferr lost; we return the original 404 err

If the fallback also fails (say the latest archive is missing the daemon binary), the user only sees "status 404 for v0.1.83" with no hint that we tried v9.9.9 too. Prefer errors.Join(err, ferr) or at least include the latest tag in the surface message.

5. commands/daemon.install.go has 0% patch coverage  _follow-up

Codecov calls this out. The orchestration (.bak restore → resolve → ensure → stale-curl cleanup) is the exact code path the bug originally hit, and none of it is exercised. A function with the filesystem-touching bits extracted (e.g. prepareDaemonBinary(ctx, baseFolder, commitID) (string, error)) would be table-testable. Not blocking this PR but worth a follow-up issue.

6. A commit SHA passed as version goes through silently  _{edge case}

normalizeDaemonTag("99d7f48") returns "v99d7f48", the download 404s, then the fallback grabs latest. Functionally fine, but the user sees a misleading first attempt. If anywhere in the build pipeline version is set to the commit SHA by mistake (it currently isn't, but the existing variable is misleadingly named commitID in the commands package), this would mask the misconfiguration. Consider rejecting non-semver-looking values up-front so they take the dev → latest path instead of the failed-tag path.

7. Tests mutate package-level globals  _nit

pointURLsAt swaps githubAPIBaseURL / githubReleaseBaseURL, and isolateDaemonFetchEnv swaps daemonHomebrewSearchPaths / daemonFetchGOOS. None of these tests use t.Parallel() today so it's fine, but it's an easy footgun if anyone adds t.Parallel() later. A short comment on each global noting "guarded by test serialization" would help.

Things done well

• The fast-path ResolveDaemonBinaryPath short-circuit means existing Homebrew/curl users pay zero network cost on reinstall.
• Cross-FS-safe moveFile in ReplaceBinary (handles the /tmp → $HOME case on Linux with separate mounts).
• safeExtractPath defends against zip-slip — important when extracting attacker-influenced archive entry names.
• The daemonFetchGOOS indirection is a clean way to exercise the Windows-aborts branch on Linux CI.
• Test fixture is well-scoped: real httptest.Server, real archive bytes, real DownloadAndVerify/ExtractBinaries path — not over-mocked.
• The 404-fallback-to-latest is exactly right for the deleted-release case.

Suggested commit message for follow-up fix

fix(model): propagate FetchChecksum non-404 errors and use sentinel for 404 fallback
· Branch